JUnit 5 migration: Replace params with subclassing #3256
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… constructor of DefaultRouteScriptAppenderTest
jcoglan
force-pushed
the
replace-params-with-subclassing
branch
from
ef8636f to
270b42c
Compare
|
We ended up resolving the failures and went ahead with this approach in #3061. I'm going to close this, and any feedback should be directed to that PR instead. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi 👋 this is James from Neighbourhoodie, working on the STF agreed milestone to migrate the test suite to JUnit 5.
This PR demonstrates an attempt to get a set of tests that use
@Parameterizedfrom JUnit 4 and try to get them running on JUnit 5. We have run into an odd problem where logger config files seem to be ignored and we're not sure how to resolve it.The class
DefaultRouteScriptAppenderTestis parameterised like so:We need to replicate the effect of this parameterisation under JUnit 5. Our understanding is that, whereas in JUnit 4 it is the constructor that is parameterised, in JUnit 5 the
@ParameterizedTestannotation applies to individual test methods.The class also uses
LoggerContextRule, and makes use of the API of that class to inspect the loggers/appenders that are configured. Our understanding is that this has been replaced with the class-level@LoggerContextSourceannotation, which causes aLoggerContextto be passed into the constructor.This PR shows an idea we had for refactoring this test class to JUnit 5. Rather than label each of its individual tests as parameterised, and then have to invoke code to set up a
LoggerContextand the necessary before/after hooks in each one, we observe that parameterisation can be achieved by subclassing. If theDefaultRouteScriptAppenderTestconstructor accepts the parameters that define each "version" of the tests, then we can write subclasses ofDefaultRouteScriptAppenderTestthat each invoke their parent's constructor with different inputs.Since the parameterisation is expressed in the form of classes, we can then also use
@LoggerContextSourceon each of those classes to turn the config filename into aLoggerContextimplicitly and not have to write/call any additional setup code ourselves.In the first commit we remove the
@Parameterized.Parametersblock and add several subclasses, each of which sets up the parent class with one of the parameter sets, e.g.This works fine and the tests continue to pass.
In the next commit, we attempt to migrate
DefaultRouteScriptAppenderTestto JUnit 5, and as part of that we removeLoggerContextRulefrom that class and addLoggerContextSourceto the subclasses:We have tried to migrate the use of
LoggerContextRulemethods likegetListAppender()to their equivalents onLoggerContext. However, these tests no longer pass; e.g. this is the result of runningmvn test -Dtest=ScriptStaticVarsJavaScriptAppenderTest:It seems as though the config filename passed to
@LoggerContextSourceis being ignored -- changing the referenced files' content has no effect on the test outcome, and using a filename for a file that does not exist does not cause a different kind of failure. It just looks like the file is ignored entirely, and we're not sure why.Is there a quirk of
@LoggerContextSourcethat means it doesn't work with subclassing, or something else we're not aware of? Should we be going about this in a different way, and if so how would you recommend turning a config filename into a workingLoggerContext?